-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
patternfly: Give empty "Th" elements a aria-label attribute #1635
patternfly: Give empty "Th" elements a aria-label attribute #1635
Conversation
As requested by this warning: Th: Table headers must have an accessible name. If the Th is intended to be visually empty, pass in screenReaderText. If the Th contains only non-text, interactive content such as a checkbox or expand toggle, pass in an aria-label. We can't use screenReaderText because of patternfly/patternfly#6643
ce7088a
to
183813d
Compare
@@ -527,7 +528,7 @@ export class VmNetworkTab extends React.Component { | |||
let networkId = 1; | |||
detailMap = detailMap.filter(d => !d.hidden); | |||
|
|||
const columnTitles = detailMap.map(target => target.name); | |||
const columnTitles = detailMap.map(target => ({ title: target.name, props: { "aria-label": target.aria } })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aria can be null here right? So we should probably not pass an aria-label which is null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's undefined
. There will be no "aria-label" attribute when aria
is undefined (or null, actually).
So... for this particular table, specifically for the icons, they're visual cues that repeat information already present in the strings (specifically, the "type" column, kind of). We should probably consider adding Also, as we're talking about accessibility in this table, it is important to include the We'd need to do something else for "empty" We'd also want Having both scopes would let someone know the context for a cell. Example from your screenshot: You'd know the "unformatted data" is a type that applies to the sda disk due to the scopes. I do see you've added the aria-label for actions, which is great, but we need more of the changes I mentioned in this comment. Thanks! |
Do you maybe mean "scope=row for |
Or, should the cells with the name be |
Scope can be added to Example from https://www.w3.org/WAI/WCAG21/Techniques/html/H63: <table border="1">
<caption>Contact Information</caption>
<tr>
<td></td>
<th scope="col">Name</th>
<th scope="col">Phone#</th>
<th scope="col">Fax#</th>
<th scope="col">City</th>
</tr><tr>
<td>1.</td>
<th scope="row">Joel Garner</th>
<td>412-212-5421</td>
<td>412-212-5400</td>
<td>Pittsburgh</td>
</tr><tr>
<td>2.</td>
<th scope="row">Clive Lloyd</th>
<td>410-306-1420</td>
<td>410-306-5400</td>
<td>Baltimore</td>
</tr><tr>
<td>3.</td>
<th scope="row">Gordon Greenidge</th>
<td>281-564-6720</td>
<td>281-511-6600</td>
<td>Houston</td>
</tr>
</table> That's an old example, and it doesn't have ARIA at all (for the empty An aside... Today I learned that things can get really complex with headers referencing headers, like this example: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/th#associate_header_cells_with_other_header_cells (it uses a It's basically the Pronunciation associated with IPA and Respelling here, due to all the colspans and rowspans: Just pointing it out that tables can get wild, and thankfully what we need to do isn't so much. However, PatternFly does have examples with this concept... https://www.patternfly.org/components/table/#nested-column-headers (and they're not doing the proper |
MDN says it is deprecated. |
@garrett, I have tried to implement your suggestions here: cockpit-project/cockpit#20471 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
As requested by this warning:
We can't use screenReaderText because of